Skip to content

test+fix: expand coverage to 194 + harden 4 high-severity edge cases#28

Merged
dmartinochoa merged 4 commits into
mainfrom
test/expand-coverage
May 19, 2026
Merged

test+fix: expand coverage to 194 + harden 4 high-severity edge cases#28
dmartinochoa merged 4 commits into
mainfrom
test/expand-coverage

Conversation

@dmartinochoa
Copy link
Copy Markdown
Member

@dmartinochoa dmartinochoa commented May 19, 2026

Summary

Two unrelated workstreams folded into one PR per maintainer instruction:

  1. Test coverage 134 → 194 (the original PR scope): backstops the recent scan-workspace fix and welcome-panel UX rework with 53 new unit + integration tests, plus a refactor that extracts installInTerminal, copyInstallCommandToClipboard, and setLspReady into their own modules so they're unit-testable instead of trapped in activate().
  2. Four high-severity edge-case fixes from the niche-scenarios audit (+7 tests on top → 194 total).

High-severity fixes

# Symptom Fix
1 Welcome panel keeps showing "Scan workspace" after the LSP crashes mid-session Subscribe to client.onDidChangeState; State.Stopped flips pipelineCheck.lspReady back to false
2 disabledProviders silently misses lowercase dockerfile / jenkinsfile providerForPath's internal regex now carries the i flag
3 Activation hangs forever on misconfigured serverArgs (e.g. []) client.start() raced against a 30s timeout; on timeout we fire-and-forget client.stop() and surface the standard install-failure toast
4 Scan workspace against a dead LSP completes with a misleading "scanned N files" toast Gate the command on the new isLspReady() synchronous mirror; surface a warning toast with Install in terminal / Restart language server / Open server log actions. Quiet mode (scan-on-save) stays silent.

Test coverage (134 → 194)

Surface Before After
findScannableFiles (the nested-brace bug path) 0 7
scanWorkspace orchestration 1 12 (incl. 3 new LSP-not-ready gate tests)
installInTerminal + copyInstallCommandToClipboard 0 10
setLspReady / isLspReady 0 7 (2 new for the readback contract)
registerStatusBar visibility latch 0 8
Manifest invariants 1 +14
providerForPath case-insensitive match 0 +2
Integration: real pipelineCheck.scanWorkspace 0 1

The shared test stub (src/__testStubs__/vscode.ts) gained findFiles, createTerminal, createStatusBarItem, openTextDocument, clipboard, setStatusBarMessage, withProgress, configurable workspaceFolders, and a resetStubState() helper.

Roadmap reconcile

ROADMAP.md was contradicting reality (status snapshot said "v0.1.1 → v0.2.0 (in flight)" with v1.0.0 already shipped; integration-test bullet marked [ ] while R17 said landed in PR #14). Now consistent: v1.0.0 shipped, integration tests done, historical manual-smoke items cleared by the marketplace release going without a regression report.

Test plan

  • npm test — 194/194 vitest assertions pass
  • npm run typecheck — clean
  • npm run lint — clean
  • npm run bundle:dev — bundle builds (808 kB)
  • tsc -p tsconfig.integration.json --noEmit — integration tests typecheck
  • CI integration suite passes (end-to-end VS Code with the new scanWorkspace assertion)

🤖 Generated with Claude Code

dmartinochoa and others added 2 commits May 19, 2026 19:30
extension.ts has been growing a long tail of one-off helpers — the
install-in-terminal flow, the clipboard-fallback copier, and the
`pipelineCheck.lspReady` context-key setter — all anchored to that
file because they read or write vscode-namespace APIs. Each one is
small but they collectively obscure the extension's main job
(activation orchestration), and none of them were unit-testable while
they lived inside the activate() closure.

Extract three things:
  - install.ts: `installInTerminal`, `copyInstallCommandToClipboard`,
    `PIP_INSTALL_COMMAND`. Both the welcome-panel CTA and the LSP-
    failure toast now share one implementation per CTA.
  - lspState.ts: `setLspReady` + the `LSP_READY_CONTEXT_KEY` constant
    that drives the conditional viewsWelcome entries. Single writer,
    single key name, single hop to VS Code's setContext channel.
  - workspaceScan.ts now exports `findScannableFiles` so the unit
    suite can pin the per-pattern findFiles contract that fences
    against the nested-brace bug returning.

No behaviour change. extension.ts shrinks; the new modules are pure
and unit-testable. Follow-up commit wires up the tests.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Backstop the recent fixes and add coverage for surfaces that had none.
The most load-bearing additions:

  - findScannableFiles: per-pattern findFiles, dedupe-on-toString,
    first-seen order. Locks the fence against the nested-brace glob
    that produced "no scannable files found" returning.
  - scanWorkspace orchestration: no-workspace path, no-files path,
    quiet vs notification toasts, openTextDocument-failure counting,
    user cancellation. Previously only formatSummary had tests.
  - installInTerminal: opens a terminal, types pip command, asserts
    addNewLine=false so the user's unactivated venv isn't violated.
    copyInstallCommandToClipboard: clipboard write + status-bar
    confirmation TTL. The PIP_INSTALL_COMMAND literal is pinned.
  - setLspReady: exact setContext payload for both true and false
    transitions. The conditional viewsWelcome panel rides on this.
  - registerStatusBar visibility latch: hidden until either a CI
    candidate file or a pipeline-check diagnostic is observed; non-
    pipeline-check sources don't flip the latch.
  - manifest.test.ts: viewsWelcome shape (two gated entries, primary
    CTAs, no "Copy install command" button), every welcome-link
    command target declared, workspace-trust capability locked.
  - Integration: runs the actual pipelineCheck.scanWorkspace command
    against the fixture workspace so a real VS Code findFiles walks
    the glob — end-to-end regression fence for the nested-brace bug.

Shared infra: the __testStubs__/vscode.ts module gains findFiles,
createTerminal, createStatusBarItem, openTextDocument, clipboard,
setStatusBarMessage, withProgress, configurable workspaceFolders, and
a resetStubState() helper that beforeEach hooks use to keep each
test's globalThis observations isolated.

134 → 187 vitest assertions; lint, typecheck, integration typecheck,
and bundle all green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 19, 2026

📝 Walkthrough

Walkthrough

This PR enhances the VS Code test infrastructure and refactors LSP installation logic: the test stub now provides observable call recording and state management; LSP readiness and pip installation helpers are extracted into dedicated modules; extension.ts is simplified to delegate responsibilities; and comprehensive unit and integration tests validate manifest contracts, status bar visibility, workspace scanning, and installation flows.

Changes

Test Infrastructure and Module Refactoring

Layer / File(s) Summary
Enhanced VS Code Test Stub
src/__testStubs__/vscode.ts
Expands stub API with exported call-shape interfaces (StubFindFilesCall, StubTerminalCall, StubExecuteCommandCall, StubClipboardWrite, StubStatusBarMessage, StubStatusBarItem) and global state slots for config, diagnostics, workspace folders, progress cancellation, and recorded calls. Implements resetStubState() to clear all global state. Enriches workspace/languages/commands/window/env implementations to record calls and read from global overrides.
LSP Readiness State Module
src/lspState.ts, src/lspState.test.ts
Exports LSP_READY_CONTEXT_KEY ("pipelineCheck.lspReady") constant and setLspReady(ready: boolean) that delegates to vscode.commands.executeCommand("setContext", ...). Test suite validates context key literal, single setContext invocations per call with correct arguments, repeated transitions, and exclusive command invocation.
Installation Helpers Module
src/install.ts, src/install.test.ts
Exports PIP_INSTALL_COMMAND and two functions: installInTerminal() creates/shows a terminal and sends pip install text without auto-enter; copyInstallCommandToClipboard() writes to clipboard and shows status-bar confirmation with 1-10s TTL. Test suite validates exact install command string, terminal focus/single-command behavior, clipboard isolation, status message presence and timing.
Extension Refactoring
src/extension.ts
Removes in-file implementations of LSP_READY_CONTEXT_KEY, setLspReady, PIP_INSTALL_COMMAND, and installInTerminal. Imports setLspReady from ./lspState and installInTerminal/copyInstallCommandToClipboard from ./install. Simplifies pipelineCheck.installInTerminal and pipelineCheck.copyInstallCommand command registrations to pass imported function references instead of inline handlers.
Manifest and Installation Contract Tests
src/manifest.test.ts
Validates package.json: pipelineCheck.findings viewsWelcome has exactly two entries gated by LSP_READY_CONTEXT_KEY vs negation; pipelineCheck.installInTerminal and pipelineCheck.copyInstallCommand commands are declared; all command:pipelineCheck.\* links in welcome contents have declared targets; workspace-trust capabilities are set to "limited" and false respectively.
Status Bar Visibility and Diagnostics Integration
src/statusBar.test.ts
Uses shared vscode stub with resetStubState() and uri() helper. Extends tooltip tests to verify Alt+F8 hint appears when findings exist. Rewrites pickBackgroundColor tests to call directly and assert ThemeColor.id. Adds registerStatusBar visibility suite: validates hidden/visible state driven by __stubFindFiles and __stubDiagnostics, filters non-pipeline-check diagnostics, asserts item text/tooltip/background color/command/menu/subscriptions registration.
Workspace Scanning File Discovery and Coverage
src/workspaceScan.ts, src/workspaceScan.test.ts
Exports findScannableFiles() with documentation of per-pattern deduping contract. Expands test suite with fakeUri helper and comprehensive coverage: findScannableFiles validates per-pattern queries, exclude-glob passthrough, result unioning, URI deduplication, first-seen ordering, and empty-pattern behavior. Adds scanWorkspace tests for no-workspace, no-scannable-files, and scanning paths including openTextDocument rejection and cancellation handling.
Extension Activation Integration Tests
src/test/integration/activation.test.ts
Adds three integration tests: verifies pipelineCheck.scanWorkspace finds fixture workflow via independent findFiles; asserts pipelineCheck.serverCommand/serverArgs remain machine-overridable for workspace-trust safety; validates viewsWelcome contributes exactly two pipelineCheck.findings entries gated on LSP_READY_CONTEXT_KEY and negation.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • greylag-ci/pipeline-check-vscode#19: Both PRs extend workspace scanning test coverage; main PR exports findScannableFiles() and tests file discovery paths while related PR covers scanWorkspace workflow orchestration.
  • greylag-ci/pipeline-check-vscode#16: Main PR's extension.ts refactoring extracts install command registration logic that complements the related PR's initial install-behavior implementation in src/extension.ts.

Poem

🐰 A stub that records, a test so complete,
Install flows extracted, both copy and seat,
LSP context shines bright, no more in-file doubt,
Manifest contracts dance, and visibility's out!
The workspace now scans with confidence true.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: expanding test coverage (from 134 to 187 assertions) and hardening edge cases through extraction and testing of critical modules.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch test/expand-coverage

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (3)
src/manifest.test.ts (1)

150-151: ⚡ Quick win

Broaden command-link target parsing to avoid brittle false failures.

At Line 150, pipelineCheck\.[A-Za-z]+ is too narrow and can truncate valid command IDs (for example IDs with extra dots/digits). Widening the capture keeps this contract test future-proof.

Suggested diff
-      const targets = [...w.contents.matchAll(/command:(pipelineCheck\.[A-Za-z]+)/g)]
+      const targets = [
+        ...w.contents.matchAll(/command:(pipelineCheck\.[A-Za-z0-9._-]+)/g),
+      ]
         .map((m) => m[1]);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/manifest.test.ts` around lines 150 - 151, The current extraction for
targets in the test uses a too-narrow regex `command:(pipelineCheck\.[A-Za-z]+)`
which can truncate valid command IDs; update the regex used when building
`targets` (the matchAll call that currently uses `pipelineCheck\.[A-Za-z]+`) to
allow digits, additional dots, underscores, and hyphens (for example
`command:(pipelineCheck[^\s)]+)` or `command:(pipelineCheck\.[\w.-]+)`) so the
full command-id is captured; keep the `.map((m) => m[1])` logic unchanged.
src/statusBar.test.ts (2)

298-301: ⚡ Quick win

Avoid sharing a mutable ExtensionContext across tests.

A module-level ctx lets subscriptions accumulate between test cases, which can mask cleanup regressions and introduce cross-test coupling. Create a fresh context per test (or in beforeEach).

♻️ Suggested change
-  const ctx = {
-    subscriptions: [] as Array<{ dispose?: () => void }>,
-  } as unknown as import("vscode").ExtensionContext;
+  let ctx: import("vscode").ExtensionContext;
+
+  beforeEach(() => {
+    ctx = {
+      subscriptions: [] as Array<{ dispose?: () => void }>,
+    } as unknown as import("vscode").ExtensionContext;
+  });
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/statusBar.test.ts` around lines 298 - 301, The test file currently
defines a module-level ctx object (variable ctx with subscriptions: [] cast to
ExtensionContext) which is shared across tests and allows subscriptions to
accumulate; change tests to create a fresh ExtensionContext for each test (e.g.
instantiate ctx inside each test or in a beforeEach) so the subscriptions array
is reinitialized per test, and update any references to the module-level ctx
variable to use the per-test/local ctx instead to avoid cross-test coupling and
hidden cleanup regressions.

386-398: ⚡ Quick win

Strengthen the cleanup assertion to verify the status-bar item is actually subscribed.

The current subs.length >= 1 can pass even if only the diagnostics disposable is registered. Assert that the created item is present in subscriptions.

✅ Suggested assertion improvement
   registerStatusBar(localCtx);
   await tick();
-  // The returned item itself + the onDidChangeDiagnostics disposable
-  // both land here. The item should be one of them.
-  expect(subs.length).toBeGreaterThanOrEqual(1);
+  const item = lastItem();
+  expect(subs).toContain(item);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/statusBar.test.ts` around lines 386 - 398, The test currently only checks
subs.length but should verify the actual status bar item was pushed; change
registerStatusBar to return the created StatusBarItem (or expose it) and update
the test to capture that return value (e.g. const item =
registerStatusBar(localCtx)) then assert subs.includes(item) (or find the exact
object in subs by identity) so you confirm the status bar item itself was added
to localCtx.subscriptions; reference registerStatusBar, localCtx and subs when
locating code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/manifest.test.ts`:
- Around line 150-151: The current extraction for targets in the test uses a
too-narrow regex `command:(pipelineCheck\.[A-Za-z]+)` which can truncate valid
command IDs; update the regex used when building `targets` (the matchAll call
that currently uses `pipelineCheck\.[A-Za-z]+`) to allow digits, additional
dots, underscores, and hyphens (for example `command:(pipelineCheck[^\s)]+)` or
`command:(pipelineCheck\.[\w.-]+)`) so the full command-id is captured; keep the
`.map((m) => m[1])` logic unchanged.

In `@src/statusBar.test.ts`:
- Around line 298-301: The test file currently defines a module-level ctx object
(variable ctx with subscriptions: [] cast to ExtensionContext) which is shared
across tests and allows subscriptions to accumulate; change tests to create a
fresh ExtensionContext for each test (e.g. instantiate ctx inside each test or
in a beforeEach) so the subscriptions array is reinitialized per test, and
update any references to the module-level ctx variable to use the per-test/local
ctx instead to avoid cross-test coupling and hidden cleanup regressions.
- Around line 386-398: The test currently only checks subs.length but should
verify the actual status bar item was pushed; change registerStatusBar to return
the created StatusBarItem (or expose it) and update the test to capture that
return value (e.g. const item = registerStatusBar(localCtx)) then assert
subs.includes(item) (or find the exact object in subs by identity) so you
confirm the status bar item itself was added to localCtx.subscriptions;
reference registerStatusBar, localCtx and subs when locating code to change.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: cb15b97c-5bdd-4670-be8e-b767beeafe65

📥 Commits

Reviewing files that changed from the base of the PR and between dcf07a0 and 50e1929.

📒 Files selected for processing (11)
  • src/__testStubs__/vscode.ts
  • src/extension.ts
  • src/install.test.ts
  • src/install.ts
  • src/lspState.test.ts
  • src/lspState.ts
  • src/manifest.test.ts
  • src/statusBar.test.ts
  • src/test/integration/activation.test.ts
  • src/workspaceScan.test.ts
  • src/workspaceScan.ts

dmartinochoa and others added 2 commits May 19, 2026 19:51
…landed

- status snapshot: mark v0.1.1, v0.2.0, v1.0.0 as shipped (was 'in flight'); add Post-1.0.0 row pointing at scan-workspace fix, welcome-panel rework, serialize-javascript override, and open PRs
- Tests section: integration tests bullet flipped to done with back-ref to R17/PR #14 and the src/test/integration/activation.test.ts surface
- C1 / H4 / maintainer-item-4 manual smokes: marked done; v1.0.0 has been live on the marketplace without a regression report, so the hypothesis is moot
1. LSP crash mid-session no longer leaves the welcome panel showing
   'Scan workspace' against a dead server. Subscribe to onDidChangeState
   so State.Stopped flips pipelineCheck.lspReady back to false; the
   install-prompt welcome state returns automatically.
2. providerForPath now matches case-insensitively, so a workspace with
   lowercase 'dockerfile' or 'jenkinsfile' is classified correctly and
   the disabledProviders middleware can actually silence it. Affects
   Windows case-preserving filesystems and any user who lowercased the
   file. Globs themselves stay POSIX-shaped.
3. client.start() is raced against a 30-second timeout. Previously an
   empty pipelineCheck.serverArgs (or any hung interpreter) left
   activation pending forever and the welcome panel stuck on the
   install prompt. On timeout we fire-and-forget client.stop() and
   surface the standard 'Install in terminal / Open server log' toast.
4. scanWorkspace gates on isLspReady() (new synchronous mirror of the
   context key) and routes the user toward Install / Restart / Show
   log via a warning toast when the LSP is down. Previously the scan
   would openTextDocument every candidate file and complete with a
   misleading 'scanned N files' toast even though no LSP was alive to
   produce findings. Quiet mode (scan-on-save) stays silent.

Tests: +7 (194 total). Adds coverage for the isLspReady readback, the
lowercase/mixed-case provider match, and the LSP-not-ready scan gate
(zero counts, no findFiles call, warning toast in noisy mode, silence
in quiet mode).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@dmartinochoa dmartinochoa changed the title test: expand coverage to 187 (was 134) test+fix: expand coverage to 194 + harden 4 high-severity edge cases May 19, 2026
@dmartinochoa dmartinochoa merged commit f9b5a8f into main May 19, 2026
12 checks passed
@dmartinochoa dmartinochoa deleted the test/expand-coverage branch May 19, 2026 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant